- 
                Notifications
    
You must be signed in to change notification settings  - Fork 421
 
          Validate channel_update signatures without holding a graph lock
          #3310
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
  
    Validate channel_update signatures without holding a graph lock
  
  #3310
              Conversation
      
          
      
      
            TheBlueMatt
  
      
      
      commented
        Sep 11, 2024 
      
    
  
In the next commit we'll move to checking `channel_update`s in three steps - first we check if the `channel_update` is new and the latest for a channel, then we check the signature, and finally we update our local state. This allows us to avoid holding a lock on `NetworkGraph::channels` while validating the message signature. Here we do a quick prefactor to make that simpler - moving the validation logic of `channel_update` that we'll do in step one (and repeat in step three) into a local function. We also take this opportunity to do one static check unlocked which we had been doing while holding a `channel` lock.
          Codecov ReportAttention: Patch coverage is  
 
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #3310      +/-   ##
==========================================
+ Coverage   89.64%   90.97%   +1.32%     
==========================================
  Files         126      126              
  Lines      102251   112645   +10394     
  Branches   102251   112645   +10394     
==========================================
+ Hits        91666   102479   +10813     
+ Misses       7863     7576     -287     
+ Partials     2722     2590     -132     
 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.  | 
    
| if msg.channel_flags & 1 == 1 { | ||
| if let Some(sig) = sig { | ||
| secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_two.as_slice()).map_err(|_| LightningError{ | ||
| let node_pubkey; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
let node_pubkey = {
	// ...
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just find that hard to read across a long block...I know we do it a lot of places, was just trying to avoid it and try something new 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, YMMV. Just seemed shorter after your change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least I didn't suggest a long chaining expression. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it affects legibility much, though consistency would favor @jkczyz's suggestion.
| 
           Looks good, feel free to squash the fixup.  | 
    
We often process many gossip messages in parallel across different peer connections, making the `NetworkGraph` mutexes fairly contention-sensitive (not to mention the potential that we want to send a payment and need to find a path to do so). Because we need to look up a node's public key to validate a signature on `channel_update` messages, we always need to take a `NetworkGraph::channels` lock before we can validate the message. For simplicity, and to avoid taking a lock twice, we'd always validated the `channel_update` signature while holding the same lock, but here we address the contention issues by doing a `channel_update` validation in three stages. First we take a read lock on `NetworkGraph::channels` and check if the `channel_update` is new, then release the lock and validate the message signature, and finally take a write lock, (re-check if the `channel_update` is new) and update the graph.
| 
           Squashed.  | 
    
0325c14    to
    131849f      
    Compare